Skip to content

Conversation

@Meinersbur
Copy link
Member

@Meinersbur Meinersbur commented Feb 3, 2025

Instead of relying on any pass manager to schedule Polly's passes, add Polly's own pipeline manager which is seen as a monolithic pass in LLVM's pass manager. Polly's former passes are now phases of the new PhaseManager component.

Relying on LLVM's pass manager (the legacy as well as the New Pass Manager1) to manage Polly's phases never was a good fit that the PhaseManager resolves:

  • Polly passes were modifying analysis results, in particular RegionInfo and ScopInfo. This means that there was not just one unique and "definite" analysis result, the actual result depended on which analyses ran prior, and the pass manager was not allowed to throw away cached analyses or prior SCoP optimizations would have been forgotten. The LLVM pass manger's persistance of analysis results is not contractual but designed for caching.

  • Polly depends on a particular execution order of passes and regions (e.g. regression tests, invalidation of consecutive SCoPs). LLVM's pass manager does not guarantee any excecution order.

  • Polly does not completely preserve DominatorTree, RegionInfo, LoopInfo, or ScalarEvolution, but only as-needed for Polly's own uses. Because the ScopDetection object stores references to those analyses, it still had to lie to the pass manager that they would be preserved, or the pass manager would have released and recomputed the invalidated analysis objects that ScopDetection/ScopInfo was still referencing. To ensure that no non-Polly pass would see these not-completely-preserved analyses, all analyses still had to be thrown away after the ScopPassManager, respectively with a BarrierNoopPass in case of the LPM.

  • The NPM's PassInstrumentation wraps the IR unit into an llvm::Any object, but implementatons such as PrintIRInstrumentation call llvm_unreachable on encountering an unknown IR unit, such as SCoPs, with no extension points to add support. Hence LLVM crashes when dumping IR between SCoP passes (such as -print-before-changed with Polly being active).

The new PhaseManager uses some command line options that previously belonged to Polly's legacy passes, such as -polly-print-detect (so the option will continue to work). Hence the LPM support is incompatible with the new approach and support for it is removed.

PRs from this patch series:

Footnotes

  1. When Chandler Carruth announced working on the NPM, I talked to him about supporting Polly's use cases. His response was "passes must adhere to the constraints of the pass manager".

@kartcq
Copy link
Contributor

kartcq commented Feb 3, 2025 via email

@Meinersbur
Copy link
Member Author

Meinersbur commented Feb 3, 2025

FYI, @rahulana-quic @tobiasgrosser

For some reason I cannot add you as reviewers, but feel free to start a review.

@Meinersbur Meinersbur marked this pull request as ready for review February 3, 2025 15:01
@kartcq
Copy link
Contributor

kartcq commented Feb 12, 2025

Hi @Meinersbur
As the change is huge, please give us some time to try it out downstream before merging.
Thanks.

@Meinersbur
Copy link
Member Author

OK, please signal your LGTM when you are ready

@kartcq
Copy link
Contributor

kartcq commented Apr 9, 2025

Thanks for your patience @Meinersbur.
I am posting some of my observations as review comments.

@Meinersbur Meinersbur marked this pull request as draft May 17, 2025 22:27
@Meinersbur Meinersbur marked this pull request as ready for review October 16, 2025 08:40
Meinersbur added a commit that referenced this pull request Oct 31, 2025
Use a common function for the non-boilerplate code shared between LPM
and NPM as done by most other passes already. ScalarEvolution is not
actually used.

Patch extracted out of #125442 requested by
#125442 (comment)
Base automatically changed from users/meinersbur/polly_CodePreparation to main October 31, 2025 18:23
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Oct 31, 2025
…M (#140419)

Use a common function for the non-boilerplate code shared between LPM
and NPM as done by most other passes already. ScalarEvolution is not
actually used.

Patch extracted out of #125442 requested by
llvm/llvm-project#125442 (comment)
DEBADRIBASAK pushed a commit to DEBADRIBASAK/llvm-project that referenced this pull request Nov 3, 2025
)

Use a common function for the non-boilerplate code shared between LPM
and NPM as done by most other passes already. ScalarEvolution is not
actually used.

Patch extracted out of llvm#125442 requested by
llvm#125442 (comment)
@Meinersbur Meinersbur merged commit e987ab1 into main Nov 3, 2025
11 checks passed
@Meinersbur Meinersbur deleted the users/meinersbur/polly_PhaseManager branch November 3, 2025 22:34
boomanaiden154 added a commit that referenced this pull request Nov 4, 2025
This reverts commit e987ab1.

This broke premerge:
1. https://lab.llvm.org/staging/#/builders/192/builds/9521
2. https://github.com/llvm/llvm-project/actions/runs/19054182009

Notably this did not break inside the PR. Not exactly sure why. I realize that
there is a lot of test churn here, but they're largely in polly where commit
frequency is much lower, so a reapply of the patch should be clean.
@boomanaiden154
Copy link
Contributor

I've reverted this in a22d1c2. It broke premerge after landing. It did not break inside this PR, likely because LLVM is not tested on polly changes.

Sorry for the churn. I know there's a lot of changed files here. Hopefully it's not too big of a deal given polly's commit frequency is lower than most of the rest of the project.

@Meinersbur
Copy link
Member Author

Meinersbur commented Nov 4, 2025

Sorry for that. I tested it locally and it did pass. I was also taking a look on the various Polly buildbot configurations (https://lab.llvm.org/buildbot/#/workers/28, https://lab.llvm.org/buildbot/#/workers/74) but unknowingly to me, their sponsor retired them.

ckoparkar pushed a commit to ckoparkar/llvm-project that referenced this pull request Nov 6, 2025
)

Use a common function for the non-boilerplate code shared between LPM
and NPM as done by most other passes already. ScalarEvolution is not
actually used.

Patch extracted out of llvm#125442 requested by
llvm#125442 (comment)
Meinersbur added a commit to Meinersbur/llvm-project that referenced this pull request Nov 11, 2025
Instead of relying on any pass manager to schedule Polly's passes, add
Polly's own pipeline manager which is seen as a monolithic pass in
LLVM's pass manager. Polly's former passes are now phases of the new
PhaseManager component.

Relying on LLVM's pass manager (the legacy as well as the New Pass
Manager) to manage Polly's phases never was a good fit that the
PhaseManager resolves:

* Polly passes were modifying analysis results, in particular RegionInfo
and ScopInfo. This means that there was not just one unique and
"definite" analysis result, the actual result depended on which analyses
ran prior, and the pass manager was not allowed to throw away cached
analyses or prior SCoP optimizations would have been forgotten. The LLVM
pass manger's persistance of analysis results is not contractual but
designed for caching.

* Polly depends on a particular execution order of passes and regions
(e.g. regression tests, invalidation of consecutive SCoPs). LLVM's pass
manager does not guarantee any excecution order.

* Polly does not completely preserve DominatorTree, RegionInfo,
LoopInfo, or ScalarEvolution, but only as-needed for Polly's own uses.
Because the ScopDetection object stores references to those analyses, it
still had to lie to the pass manager that they would be preserved, or
the pass manager would have released and recomputed the invalidated
analysis objects that ScopDetection/ScopInfo was still referencing. To
ensure that no non-Polly pass would see these not-completely-preserved
analyses, all analyses still had to be thrown away after the
ScopPassManager, respectively with a BarrierNoopPass in case of the LPM.
 
* The NPM's PassInstrumentation wraps the IR unit into an `llvm::Any`
object, but implementations such as PrintIRInstrumentation call
llvm_unreachable on encountering an unknown IR unit, such as SCoPs, with
no extension points to add support. Hence LLVM crashes when dumping IR
between SCoP passes (such as `-print-before-changed` with Polly being
active).

The new PhaseManager uses some command line options that previously
belonged to Polly's legacy passes, such as `-polly-print-detect` (so the
option will continue to work). Hence the LPM support is incompatible
with the new approach and support for it is removed.
Meinersbur added a commit that referenced this pull request Nov 13, 2025
)

Reapply of a22d1c2. Using this PR for
pre-merge CI.

Instead of relying on any pass manager to schedule Polly's passes, add
Polly's own pipeline manager which is seen as a monolithic pass in
LLVM's pass manager. Polly's former passes are now phases of the new
PhaseManager component.

Relying on LLVM's pass manager (the legacy as well as the New Pass
Manager) to manage Polly's phases never was a good fit that the
PhaseManager resolves:

* Polly passes were modifying analysis results, in particular RegionInfo
and ScopInfo. This means that there was not just one unique and
"definite" analysis result, the actual result depended on which analyses
ran prior, and the pass manager was not allowed to throw away cached
analyses or prior SCoP optimizations would have been forgotten. The LLVM
pass manger's persistance of analysis results is not contractual but
designed for caching.

* Polly depends on a particular execution order of passes and regions
(e.g. regression tests, invalidation of consecutive SCoPs). LLVM's pass
manager does not guarantee any excecution order.

* Polly does not completely preserve DominatorTree, RegionInfo,
LoopInfo, or ScalarEvolution, but only as-needed for Polly's own uses.
Because the ScopDetection object stores references to those analyses, it
still had to lie to the pass manager that they would be preserved, or
the pass manager would have released and recomputed the invalidated
analysis objects that ScopDetection/ScopInfo was still referencing. To
ensure that no non-Polly pass would see these not-completely-preserved
analyses, all analyses still had to be thrown away after the
ScopPassManager, respectively with a BarrierNoopPass in case of the LPM.
 
* The NPM's PassInstrumentation wraps the IR unit into an `llvm::Any`
object, but implementations such as PrintIRInstrumentation call
llvm_unreachable on encountering an unknown IR unit, such as SCoPs, with
no extension points to add support. Hence LLVM crashes when dumping IR
between SCoP passes (such as `-print-before-changed` with Polly being
active).

The new PhaseManager uses some command line options that previously
belonged to Polly's legacy passes, such as `-polly-print-detect` (so the
option will continue to work). Hence the LPM support is incompatible
with the new approach and support for it is removed.
Meinersbur added a commit that referenced this pull request Nov 16, 2025
PR #125442 replaces the pass-based Polly architecture with a monolithic
pass consisting of phases. Reasons listed in
#125442.

With this change, the SCoP-passes became redundant problematic versions
of the same functionality and are removed.
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Nov 16, 2025
PR #125442 replaces the pass-based Polly architecture with a monolithic
pass consisting of phases. Reasons listed in
llvm/llvm-project#125442.

With this change, the SCoP-passes became redundant problematic versions
of the same functionality and are removed.
nekoshirro pushed a commit to nekoshirro/Alchemist-LLVM that referenced this pull request Nov 24, 2025
PR #125442 replaces the pass-based Polly architecture with a monolithic
pass consisting of phases. Reasons listed in
llvm/llvm-project#125442.

With this change, the SCoP-passes became redundant problematic versions
of the same functionality and are removed.
Signed-off-by: Hafidz Muzakky <[email protected]>
aadeshps-mcw pushed a commit to aadeshps-mcw/llvm-project that referenced this pull request Nov 26, 2025
PR llvm#125442 replaces the pass-based Polly architecture with a monolithic
pass consisting of phases. Reasons listed in
llvm#125442.

With this change, the SCoP-passes became redundant problematic versions
of the same functionality and are removed.
google-yfyang pushed a commit to google-yfyang/llvm-project that referenced this pull request Dec 1, 2025
)

Use a common function for the non-boilerplate code shared between LPM
and NPM as done by most other passes already. ScalarEvolution is not
actually used.

Patch extracted out of llvm#125442 requested by
llvm#125442 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants